⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 46.20%. Comparing base (8691287) to head (302d545).
Report is 10 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #5865      +/-   ##
==========================================
- Coverage   46.32%   46.20%   -0.13%     
==========================================
  Files         661      663       +2     
  Lines       54400    54598     +198     
==========================================
+ Hits        25200    25225      +25     
- Misses      27576    27751     +175     
+ Partials     1624     1622       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

Currently, the rb/crb status controller watchs rb/crb and work, but the predicates for creating events are all false, which means that if the work status is up to date, but the queue of the rb/crb status controller is not cleared, and it crashes, these unprocessed items will not be requeued after the next restart, and the status of rb/crb and workload will not be updated

Another approach is letting the resource-binding-status-controller respond to the Work creation event. See the code here:

Which approach do you prefer? @zach593 @chaunceyjiang @XiShanYongYe-Chang

I think this is some kind of self-checking of rb/crb, and from this point of view, introducing more interdependencies between objects is unnecessary.
So I prefer the rb/crb creation event. @RainbowMango

letting the resource-binding-status-controller respond to the Work creation event.

I agree with this approach.

from this point of view, introducing more interdependencies between objects is unnecessary.

Hi @zach593, Are you referring to the concern that duplicate rb/crbs are being processed?

I understand that we have increased attention to the create event, the only reason is to worry about controller restart, am I right?

In my opinion, both approaches should work, considering that the number of ResourceBinding is less than the number of Works, I prefer the first one(the current PR approaches).

@XiShanYongYe-Chang Can you explain why you prefer the second approach?

@zach593 Do you think this patch would have an impact on performance?

Are you referring to the concern that duplicate rb/crbs are being processed?

No, unually extra reconciliation should not be harmful, unless it runs into some performance issues.

I understand that we have increased attention to the create event, the only reason is to worry about controller restart, am I right?

Yes, this PR is mainly focused on controller restarts

@XiShanYongYe-Chang

Do you think this patch would have an impact on performance?

After #5779, I think it should not. I remember in the first test report in #5779 (comment), I added a test that with this predicate return true, and basically this predicate caused no impact.

@RainbowMango

I end up leaning towards this way:
letting the resource-binding-status-controller respond to the RB creation event. this allows for as little reconcile as possible on the basis of solving problems.

I think this is some kind of self-checking of rb/crb, and from this point of view, introducing more interdependencies between objects is unnecessary.

But I still don't understand the meaning of the second half of the sentence.

Our goal is to add a self-check mechanism to the status of RB. From a design perspective, when faced with multiple options, if one option can reduce dependencies on other resources, it helps clarify the boundaries between subsystems, achieving what is known as "low in coupling and high in cohesion." From this perspective, the first option is preferable.
Additionally, from a practical impact perspective, this approach also introduces fewer reconciliation operations, which is even better. @XiShanYongYe-Chang

Do we need a release note and cherry-pick it to the previous branch?

Do we need a release note and cherry-pick it to the previous branch?

No strong opinion from me. @zach593 what do you say?

I added a release notes, by the way.

cool, lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Do you think cherry-pick is needed?

According to cherry-pick, the current pr meets the standard, let me build an issue to track it.